Rust, shared: Support Parameter in source MaD models
#20452
Conversation
github-actions
bot
added
Rust
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
7bbf6f7 to
e3c9f79
Compare
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
e3c9f79 to
e6bd0ed
Compare
paldepind
force-pushed
the
rust/mad-source-parameter
branch
2 times, most recently
from
bb712f0 to
99e88c5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for MaD (Model-as-Data) sources with Argument[_].Parameter[_] access paths in Rust. This enables modeling of sources that pass tainted data to callbacks or function parameters.
Key changes:
- Updated the shared dataflow framework to support
Parametercomponents in source access paths - Modified flow summary implementations across multiple languages to use
SummaryComponentStackinstead ofSummaryComponent - Added jump steps for sources that cross callable boundaries
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll |
Core implementation supporting Parameter in source models with jump steps |
rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll |
Rust-specific implementation for handling callback parameter sources |
rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll |
Integration of source jump steps in Rust dataflow |
rust/ql/test/library-tests/dataflow/models/models.ext.yml |
Added test model for pass_source function |
| Multiple language-specific files | Updated signatures to use SummaryComponentStack |
| DataFlowCall getACall(SummarizedCallable sc); | ||
|
|
||
| /** Gets the enclosing callable of `source`. */ | ||
| DataFlowCallable getSourceNodeEnclosingCallable(SourceBase source); |
There was a problem hiding this comment.
We need to know wether the node returned by getSourceNode is in the same callable as SourceBase, and I don't think we can get that information without adding a new predicate.
| --- | ||
| * The models-as-data format for sources now supports access paths of the form | ||
| `Argument[i].Parameter[j]`. This denotes that the source passes tainted data to | ||
| the `j`th parameter of it's `i`th argument (which must be a function or a |
|
|
||
| /** Get the callable that `expr` refers to. */ | ||
| private Callable getCallable(Expr expr) { | ||
| result = resolvePath(expr.(PathExpr).getPath()).(Function) |
There was a problem hiding this comment.
I wasn't aware this was possible in Rust; we don't currently handle it in normal data flow.
| arg = pos.getArgument(call) | ||
| exists(ArgumentPosition pos, Expr arg | | ||
| s.head() = Impl::Private::SummaryComponent::parameter(pos) and | ||
| arg = getSourceNodeArgument(source, s.tail().head()) and |
There was a problem hiding this comment.
We should check that s.tail() is a singleton.
There was a problem hiding this comment.
Now done, through a new headOfSingleton member predicate. I've also rebased.
| ) | ||
| or | ||
| result.(RustDataFlow::PostUpdateNode).getPreUpdateNode().asExpr().getExpr() = | ||
| getSourceNodeArgument(source, s.head()) |
There was a problem hiding this comment.
We should check that s is a singleton.
paldepind
force-pushed
the
rust/mad-source-parameter
branch
from
25c14ea to
45b84ff
Compare
|
C# CI failure appears unrelated. |
|
I just looked at the DCA results, I'm seeing a really quite big boost to taint reach now that those remote sources are working properly! I'm looking forward to this getting merged as soon as the various threads above are resolved. |
This PR adds support for MaD sources with an
Argument[_].Parameter[_]access path. This corresponds to sources that pass tainted data to a callback/function.The implementation is my interpretation of a comment on Slack by @hvitved. My understanding is that this is not necessarily the long-term ideal way to implement this, but it is a quick way to give us something that works and that lets us write the models that we want to write for Rust.
The DCA report looks good. Taint reach triples on
rust-vulnerable-appswhich makes sense as it uses Warp for which we now have working models.